Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing the signed-resource requirement for account SAS tokens #396

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

tomswedlund
Copy link

Addressing the following bug: #383

@Jinming-Hu
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@Jinming-Hu Jinming-Hu changed the base branch from master to dev May 1, 2021 03:13
@Jinming-Hu
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 396 in repo Azure/azure-storage-cpp

@tomswedlund
Copy link
Author

Hi Jinming,

Sorry for the build break, I swear I built this locally. But this brings up a different issue. Should the conditional that I removed actually be removed, or should we be passing false to the function in case an account SAS is being used? Looks like "require_signed_resource" is only set to true in two places:

  • src/cloud_file_directory.cpp, line 72
  • src/cloud_file.cpp, line 92

Should one of these (or both) be set to false instead?

Tom

@Jinming-Hu
Copy link
Member

@tomswedlund Hi, thanks for your contribution. I believe the CI failure has nothing to do with the change in this PR. I'm gonna take some time to fix Azure pipelines.

@tomswedlund
Copy link
Author

HI Jinming,

I see a build failure with the following error:
microsoft.windowsazure.storage\src\shared_access_signature.cpp(85,0): Error C4100: 'require_signed_resource': unreferenced formal parameter

Looks like the 'require_signed_resource' function param is no longer being used because of my change, and hence the error. Should this parameter be removed altogether? Or should we passing 'false' in the two places I mentioned in my last comment?

Thanks,
Tom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants